fix snapshots, add macOS docker support #272
Conversation
|
Unable to perform a code review. You have run out of credits 😔 |
Reviewer's GuideThis PR refactors command dispatch to handle snapshots earlier, enhances snapshot output configuration, updates installation docs with Linux and macOS Docker workflows, and adds macOS Docker support artifacts including a runner script, Dockerfile, and automated snapshot tests. Sequence diagram for early snapshot command handlingsequenceDiagram
participant User
participant CLI
participant SnapshotHandler
User->>CLI: osvm snapshot [args]
CLI->>SnapshotHandler: execute_snapshot_command(sub_matches)
SnapshotHandler-->>CLI: Result
CLI-->>User: Output
Class diagram for updated OutputConfig in snapshot commandclassDiagram
class OutputConfig {
+OutputFormat format
+bool colorized
+bool quiet
+bool show_progress
+bool human_readable
}
OutputConfig : colorized is now set by NO_COLOR env var
OutputConfig : format set by --json flag
OutputConfig : quiet set by --quiet flag
OutputConfig : show_progress set by !quiet
OutputConfig : human_readable set by !json
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In get_output_config, consider combining the NO_COLOR env var check with the --no-color flag so users can override colors via the CLI flag.
- README macOS usage example has "ovsm eval"; it should reference the correct binary name "osvm eval".
- docker-run-macos.sh mounts ~/.osvm into the container without setting a user, which can lead to permission issues—consider adding a --user flag or adjusting ownership.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In get_output_config, consider combining the NO_COLOR env var check with the --no-color flag so users can override colors via the CLI flag.
- README macOS usage example has "ovsm eval"; it should reference the correct binary name "osvm eval".
- docker-run-macos.sh mounts ~/.osvm into the container without setting a user, which can lead to permission issues—consider adding a --user flag or adjusting ownership.
## Individual Comments
### Comment 1
<location> `src/commands/snapshot.rs:184-193` </location>
<code_context>
}
fn get_output_config(matches: &ArgMatches) -> Result<OutputConfig> {
+ // Check environment variable for NO_COLOR or flag
+ let colorized = std::env::var("NO_COLOR").is_err();
+
Ok(OutputConfig {
format: if matches.get_flag("json") {
crate::services::snapshot_service::OutputFormat::Json
} else {
crate::services::snapshot_service::OutputFormat::Text
},
- colorized: !matches.get_flag("no-color"),
+ colorized,
quiet: matches.get_flag("quiet"),
show_progress: !matches.get_flag("quiet"),
</code_context>
<issue_to_address>
**suggestion:** NO_COLOR environment variable now overrides the CLI flag for colorized output.
Consider updating the logic so that either the NO_COLOR environment variable or the --no-color flag disables color output, or clarify in the documentation that NO_COLOR takes precedence over the flag.
</issue_to_address>
### Comment 2
<location> `src/commands/snapshot.rs:195` </location>
<code_context>
- colorized: !matches.get_flag("no-color"),
+ colorized,
quiet: matches.get_flag("quiet"),
show_progress: !matches.get_flag("quiet"),
human_readable: !matches.get_flag("json"),
</code_context>
<issue_to_address>
**suggestion:** show_progress is tied to the quiet flag, but not to output format.
Disabling progress when JSON output is selected will prevent issues with machine parsing. Please update the logic accordingly unless progress in JSON is required.
```suggestion
show_progress: !matches.get_flag("quiet") && !matches.get_flag("json"),
```
</issue_to_address>
### Comment 3
<location> `README.md:70` </location>
<code_context>
+
+# Use any command
+./scripts/docker-run-macos.sh snapshot --help
+./scripts/docker-run-macos.sh ovsm eval '(+ 1 2 3)'
```
</code_context>
<issue_to_address>
**issue (typo):** Possible typo: 'ovsm' should likely be 'osvm'.
Please update 'ovsm' to 'osvm' for consistency with the rest of the documentation and repository.
```suggestion
./scripts/docker-run-macos.sh osvm eval '(+ 1 2 3)'
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| colorized: !matches.get_flag("no-color"), | ||
| colorized, | ||
| quiet: matches.get_flag("quiet"), | ||
| show_progress: !matches.get_flag("quiet"), |
There was a problem hiding this comment.
suggestion: show_progress is tied to the quiet flag, but not to output format.
Disabling progress when JSON output is selected will prevent issues with machine parsing. Please update the logic accordingly unless progress in JSON is required.
| show_progress: !matches.get_flag("quiet"), | |
| show_progress: !matches.get_flag("quiet") && !matches.get_flag("json"), |
|
|
||
| # Use any command | ||
| ./scripts/docker-run-macos.sh snapshot --help | ||
| ./scripts/docker-run-macos.sh ovsm eval '(+ 1 2 3)' |
There was a problem hiding this comment.
issue (typo): Possible typo: 'ovsm' should likely be 'osvm'.
Please update 'ovsm' to 'osvm' for consistency with the rest of the documentation and repository.
| ./scripts/docker-run-macos.sh ovsm eval '(+ 1 2 3)' | |
| ./scripts/docker-run-macos.sh osvm eval '(+ 1 2 3)' |
|
I'm not naive enough to believe there's not something else we need to do outside of this granted you asked me to fix it (which I'm looking into now), but this should be fixed regardless for the random folks running this locally etc. EDIT: maybe not... works fine with or without keypair. Can you please elaborate on what the specific bug is you would like help with? |
|
Also, fwiw, I couldn't get |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Docker support for macOS users, enabling them to run OSVM CLI in a containerized environment. The changes provide a workaround for macOS users who may face native build issues while maintaining the existing Linux build workflow.
- Adds macOS-specific Docker infrastructure (Dockerfile and run script)
- Updates installation instructions in README to document both Linux (native) and macOS (Docker) installation methods
- Moves snapshot command handling earlier in main.rs execution flow to avoid unnecessary config loading
- Fixes NO_COLOR environment variable handling in snapshot command to align with standard behavior
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test-scripts/test-snapshot-docker.sh | Adds integration test for verifying snapshot commands work correctly inside Docker container |
| scripts/docker/Dockerfile-macos | New multi-stage Dockerfile for building OSVM CLI for macOS users |
| scripts/docker-run-macos.sh | Helper script to build and run OSVM in Docker with proper volume mounts for configuration |
| src/main.rs | Adds early handling of snapshot command to avoid unnecessary keypair/config loading |
| src/commands/snapshot.rs | Updates NO_COLOR environment variable handling to correctly respect the standard |
| README.md | Documents macOS Docker installation method alongside existing Linux native installation |
| @@ -0,0 +1,55 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This violates the project's "NO STANDALONE SCRIPTS POLICY". According to .github/copilot-instructions.md, all functionality must be implemented in Rust and compiled into the osvm binary. Standalone shell scripts are explicitly forbidden. Consider implementing this test functionality as a Rust integration test or as an osvm subcommand (e.g., osvm test snapshot-docker).
| @@ -0,0 +1,24 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This violates the project's "NO STANDALONE SCRIPTS POLICY". According to .github/copilot-instructions.md, all functionality must be implemented in Rust and compiled into the osvm binary. Consider implementing Docker orchestration as a Rust-based osvm subcommand (e.g., osvm docker run) that handles the container lifecycle, or document this as a temporary developer convenience script with a plan to migrate to Rust.
voidcooks
force-pushed
the
fix/snapshot-macos
branch
from
bbd6bc0 to
825d113
Compare
2 participants
Summary by Sourcery
Improve snapshot command handling, add macOS Docker support and tests, and update installation instructions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: